[.ai] add self-review skill#13917
Merged
Merged
Conversation
… the agent guides - New `self-review` skill mirroring the `@claude` CI review (rubric from review-rules.md, call-path dead-code analysis), report-only, with the report flagging what to fix before submitting (blocking + dead code) vs what to leave for the actual review. - Remove the WIP `parity-testing` skill; preserve its pitfalls as `model-integration/pitfalls.md` (numerical-discrepancy reference). - model-integration: restructure around a grouped checklist, default-to-modular, an overall file-structure sketch (details deferred to the guides), a fresh-conversion `Model parity test` example (internal, not shipped), and a filled-in weight/checkpoint-conversion section. - Centralize the loading rule (from_pretrained / from_single_file, no custom loaders) in models.md; add per-folder File structure sections to models.md / pipelines.md; default-to-modular note in pipelines.md. - AGENTS.md: dedicated 'Self-review before a PR' and 'Reference guides' sections. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t entries Trim pitfall #6 to the essential point (small dtype diffs compound into a large final difference), remove the `/tmp` model-storage and incomplete-injection-test pitfalls, and renumber 1-16 with cross-references updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
With the parity-testing skill gone, remove the stale-test-fixtures pitfall (saved tensors / cross-pipeline fixtures no longer apply) and de-jargon the noise-dtype detection note. Keeps the pitfalls list generic to numerical discrepancy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Drop the variable-shadowing and decoder-config pitfalls and the noise-dtype 'Detection' aside, tighten the remaining entries, renumber 1-12 (cross-refs updated), and reframe the intro as a non-checklist reference list of possible causes to consult only when outputs don't match. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
yiyixuxu
commented
Jun 11, 2026
| @@ -1,172 +0,0 @@ | |||
| --- | |||
| name: testing-parity | |||
Collaborator
Author
There was a problem hiding this comment.
i removed this skill for now since it is a WIP
yiyixuxu
commented
Jun 11, 2026
yiyixuxu
commented
Jun 11, 2026
yiyixuxu
commented
Jun 11, 2026
| @@ -0,0 +1,48 @@ | |||
| --- | |||
| name: self-review | |||
Collaborator
Author
There was a problem hiding this comment.
added a self-review skill here
mostly doing the samething our claude CI do, but for self-review I think they should focus on blocking issues & clean up dead code
we may ask contributor to provide the self-review summary in PR submission as well
Replace the retired parity-testing skill with self-review in the skills list, and add a 'Self-review before opening' step to the AI-assisted contributions section: run the self-review skill / review-rules, fix blocking issues + dead code, and treat the @claude CI review as a non-authoritative helper (note any intentional skips in the PR for the reviewer). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
the
documentation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
sayakpaul
approved these changes
Jun 11, 2026
| - [ ] **Scheduler** — reuse an existing scheduler, or add a custom one | ||
| - [ ] **Pipeline** | ||
| - [ ] Implement the pipeline — see [modular.md](../../modular.md) for modular pipeline, or [pipelines.md](../../pipelines.md) for standard pipeline | ||
| - [ ] Add a LoRA mixin if applicable |
Member
There was a problem hiding this comment.
Maybe better if this is skipped in the initial implementation. Easier to review and test.
Co-authored-by: Sayak Paul <spsayakpaul@gmail.com>
![sergereview[bot]](https://avatars.githubusercontent.com/in/3673521?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🤗 Serge says:
This PR adds a self-review skill, consolidates parity-testing content into the model-integration skill, and restructures several .ai/ guide files. The changes are mostly documentation reorganization. Two issues found:
Correctness
- Broken cross-reference in
SKILL.md: Line 77 referencespitfalls.md #10for the "never use/tmp/" rule, but in the newpitfalls.md, item #10 is "RoPE position dtype" — the storage-location pitfall was dropped during the consolidation. Either re-add the storage-location pitfall to the newpitfalls.md(and renumber), or inline the advice directly in the SKILL.md sentence and drop the#10reference.
Minor
- Scope claim mismatch in self-review skill: The skill header (lines 5-6) says it "Runs the same review the
@claudeCI runs", but line 17 restricts scope tosrc/diffusers/and.ai/only. The CI reviewer reviews the full diff (tests, docs, scripts, etc.). Either widen the scope or soften the claim (e.g. "Runs the same rubric" rather than "the same review").
model: claude-opus-4-6 · 17 LLM turns · 21 tool calls · 145.8s · 595532 in / 5054 out tokens
| See [../../models.md](../../models.md) for the attention pattern, implementation rules, common conventions, dependencies, and gotchas. These apply to all model work. | ||
| 1. Map the original state-dict keys to the diffusers module names (renames + any tensor surgery — see patterns below). | ||
| 2. Instantiate the diffusers model from its config and load the converted state dict. | ||
| 3. `save_pretrained(...)` to a **persistent** path (never `/tmp/` — see [pitfalls.md](pitfalls.md) #10), then load it back with `from_pretrained` to confirm it round-trips. |
Contributor
There was a problem hiding this comment.
Bug — broken #10 reference. The parenthetical says see [pitfalls.md](pitfalls.md) #10, but in the new pitfalls.md item #10 is "RoPE position dtype", not the storage-location rule. The original pitfall #10 ("NEVER store converted models in /tmp/") was dropped during the consolidation from parity-testing/pitfalls.md.
Suggested fix — inline the advice and drop the dangling anchor:
Suggested change
| 3. `save_pretrained(...)` to a **persistent** path (never `/tmp/` — see [pitfalls.md](pitfalls.md) #10), then load it back with `from_pretrained` to confirm it round-trips. | |
| 3. `save_pretrained(...)` to a **persistent** path (never `/tmp/` — temporary directories get wiped on restart), then load it back with `from_pretrained` to confirm it round-trips. |
| The same pass the `@claude` CI reviewer runs, so you can catch issues before a | ||
| maintainer does. You're already on the branch with the conventions loaded, so: | ||
| get the diff → review it against the rubric → report. **Review only changes under | ||
| `src/diffusers/` and `.ai/`** (skip everything else, like the CI does). |
Contributor
There was a problem hiding this comment.
Scope mismatch with the claim on lines 5-6 ("Runs the same review the @claude CI runs"). The CI reviewer reviews the full diff — including tests/, docs/, and scripts/ — not just src/diffusers/ and .ai/. Either widen the scope here or soften the header claim to something like "Runs the same rubric" rather than "the same review".
- Drop the broken 'pitfalls.md #10' reference in the conversion step (the /tmp model-storage pitfall was removed); save to a local path instead. - Self-review now reviews the whole diff, not just src/diffusers/ and .ai/ — a contributor should review their own tests/docs/scripts too (the CI's scoping is a safety measure for untrusted PRs). Reword to 'same rubric as the CI'. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.